#254: stop the worker from pinning every task's nested closure - #257
Merged
Conversation
DECLARE_LAMBDA_FUNCTION memoizes the Closure it creates and pins it in EG(lambda_cache), which is drained when the request ends. A worker's request outlives every task it runs, so the pinned object held the task's nested body at refcount 1 and destroy_op_array left the whole task op_array behind. Measured at about 930 bytes per task, linear: 6 MB over 6000 submits, flat after the change. Releasing the pinned closures at task end would not do: the handler reads the cache slot before creating anything, so a closure that outlived its task would hand out a freed object on the next call. The memo is turned off instead, which is what zend_compile_func_decl already does for the top level of a script, for the same reason and with the same trade: the same closure literal evaluated twice yields two objects.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The test compared the worker's memory against a reading taken 200 tasks in, so anything a worker allocates once on its way up counted against the bound, and the release build on CI crossed it. Two windows of a thousand tasks are compared instead: a one-off allocation lands in the first, a per-task leak shows in both. The numbers are printed when the bound is crossed, so a failure says how much rather than only that.
The literal allocation split by #255 and the lambda memo of this branch meet in the same block: the split stays, the memo loop moves below the memcpy that both need.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #254.
What was wrong
ZEND_DECLARE_LAMBDA_FUNCTIONmemoizes theClosureit creates: it takes a reference, writes the object into a run-time cache slot and pushes it ontoEG(lambda_cache). That stack is drained in one place,shutdown_executor, when the request ends.A pool worker runs one request for its whole life. Every task is materialized into an op_array of its own with a cache of its own, so the slot is empty at the first execution and a new Closure is pinned — once per task, for as long as the worker accepts tasks. The pinned object holds
dynamic_func_defs[i]->refcountat 1,destroy_op_arrayreturns early, and that task's opcodes, literals, vars, arg_info, attributes and static variables are never freed.Measured in the worker, task body
$f = static function () { return 1; }; return $f();:About 930 bytes per task, linear, no ceiling. Present on
mainbefore #251 with the same numbers, so it is older than that work.What changed
The memo is turned off in a materialized op_array:
extended_valueon everyZEND_DECLARE_LAMBDA_FUNCTIONis set to(uint32_t) -1, the value the compiler leaves when caching does not apply.That is the decision
zend_compile_func_declalready makes for the top level of a script, and its comment gives the same reason: "Don't cache closures in main, as those would leak without a proper cleanup mechanism." A worker's per-task op_array is the same situation. The memo buys nothing there in any case — the cache dies with the task that owns it, so nothing is ever reused.Releasing the pinned closures at task end was the other candidate and does not work: the handler reads the cache slot before it creates anything, so a closure that outlived its task would hand out a freed object on the next call.
The price
Inside a task, the same closure literal evaluated twice yields two objects; in the submitting thread it yields one. Asserted in the test rather than left to be discovered.
Verification
tests/thread_pool/102-worker_releases_nested_closures.phpt— new. Red 3 of 3 on revert, green after. It warms up 200 tasks, readsmemory_get_usage()in the worker, runs 2000 more and asserts the growth is under 100 KB; before the change that run grew by about 1.9 MB. Measured after the change: 502192 bytes on all six sample points, flat.tests1192 passed / 0 failed, with OPcache enabled as well;fuzzy-tests/_generated724 / 0.Build: ZTS DEBUG, TrueAsync ABI v0.24.0.